test/ci: enterprise-grade hardening — chaos e2e, mTLS, soak, perf trend, API gate, pipeline reliability - #52
Merged
Conversation
…nd, API gate, pipeline fixes P0 — CI pipeline reliability: - Spice CLI install steps (build x3 incl. Windows, publish, new jobs): retry with backoff + binary verification + GITHUB_TOKEN (the JDK-matrix job lacked the token and caused most installer flakes) - OWASP: step-level 20m timeout so a slow NVD refresh can't hit the job timeout (which cancels the job and bypasses continue-on-error); nvdValidForHours=24 so warm caches skip the refresh entirely P0 — chaos e2e (ChaosE2ETest + e2e-chaos job, gated by SPICE_E2E_CHAOS): manages its own spiced processes (dataset-free pod) and proves against the real runtime: crash/restart recovery on one client (reconnect + prepared statement re-prepare), queries during downtime recovering via retry backoff, clean mid-stream failure on SIGKILL, and keep-alive detection of a frozen (SIGSTOP) peer with recovery after SIGCONT. 4/4 green locally. P1 — mTLS integration (MtlsTest + TestCerts + TLS mode in TestFlightSqlServer): runtime-generated CA/server/client certificates, real handshakes in-process — custom-CA trust, full mutual TLS including parameterized queries, missing-client-cert and untrusted-CA rejection. First e2e coverage of the TLS configuration surface. P1 — japicmp API-compatibility gate vs the last release (0.6.0), wired into the quality job; verified the 0.7.0 surface is purely additive. ADBC types (internal in 0.6.0, removed since) are scoped as ignored. P1 — nightly workflow: 30-min soak (SoakTest: mixed workload with periodic reset(); zero errors, leak-free close, bounded thread growth from a warm baseline, p99 stability across the run — validated locally at ~9,200 ops/s), chaos rerun, and benchmark trend publishing to gh-pages via github-action-benchmark (warn-only alerts; PerfBenchmarkTest emits customSmallerIsBetter JSON when BENCH_JSON is set). docs/testing.md documents the tiers and how to run each locally. Full gate: 293 tests, 0 failures; SpotBugs clean.
Security (workflows): - publish.yaml: replace curl|bash with a pinned, sha256-verified download of the exact Spice CLI/runtime release, and defer the GPG key import to a dedicated step after all downloaded code has run — no mutable remote code ever executes in a release-signing context - nightly.yaml: pin github-action-benchmark to a full commit SHA (v1.22.1); document that GITHUB_TOKEN pushes don't trigger a Pages build Correctness (workflows): - build.yaml: split OWASP into a best-effort NVD update (continue-on-error) and a gating offline check — network flakiness can no longer mask real CVSS>=7 findings, and findings can no longer be swallowed as flakes - nightly.yaml: curl timeouts in the readiness loop (no wedge on a stalled endpoint) Tests: - ChaosE2ETest: fail loudly when SPICE_E2E_CHAOS=1 but spiced is missing (was a silent no-op pass); daemon hang-guard threads; kill() asserts the process actually exited; launch() destroys half-started processes instead of orphaning them; start() retries the ephemeral-port race; the downtime restarter is joined in finally and its failures propagate - SoakTest: bounded latency retention (5k samples/minute — a 30-min soak previously retained every sample and risked OOM before asserting); asserts every data operation returned rows (silent-empty-results guard) - MtlsTest: negative tests now assert the failure is a TLS/certificate failure by walking the cause chain — which immediately exposed that they had been passing via IPv6 connection-refused before any handshake; TLS server + client now bind/dial 127.0.0.1 explicitly - PerfBenchmarkTest: benchmark ops assert full row counts (a result-dropping regression can't publish an 'improved' latency) and the plain-query contract also asserts the DoGet count Accepted as-is: temp workspace/cert cleanup (throwaway 1-day self-signed fixtures in user-private OS temp on ephemeral runners). Validated: 293 tests + SpotBugs green; chaos e2e 4/4 against real spiced.
Workflows (reuse + altitude): - New composite actions .github/actions/setup-spice (retried, verified, token-authenticated CLI/runtime install — replaces six inlined copies in two languages) and start-spice-app (quickstart heredoc + /v1/ready poll — replaces four copies and retires the flaky 'sleep 10' everywhere, including the pre-existing build/publish app-start steps) - Dropped the nightly chaos job (verbatim copy of the per-push e2e-chaos) - e2e-chaos and soak jobs no longer run a full mvn install nothing consumes - quality job runs checkstyle+japicmp in one Maven session - bench-trend runs with -Djacoco.skip=true so published latencies measure the real SDK, not coverage-instrumented classes (pom gains an empty argLine default so surefire works when JaCoCo is skipped) - japicmp oldVersion hoisted to a property; the publish preflight now fails if it equals the version being released (vacuous self-comparison guard) Tests (simplification + efficiency): - SpicedProcess promoted from ChaosE2ETest inner class to a top-level package-private fixture (spicepod content parameterized) - Keep-alive interval/timeout promoted to package-visible SpiceClient constants; the frozen-peer test derives its detection bound from them - ChaosE2ETest: shared daemon guard executor, AssertionError instead of fail+unreachable-throw, frozen-test recovery restructured sequentially with an honest comment, countRows delegates to the shared helper - SoakTest: single bounded synchronizedList map replaces the parallel queue+counter maps, plain int roll counter, awaitTermination doubles as the completion join (latch removed), summary reuses p99Of - PerfBenchmarkTest: checkedCountRows + paramsOp factory replace three hand-rolled assertion lambdas; plain imports replace inline FQNs - TestCerts: EC P-256 keys (~300x faster than RSA-2048 on CI, same compatibility), writeKeyPem delegates to writePem - TestFlightSqlServer: pemStream helper + plain imports Validated: 293 tests + SpotBugs green; jacoco.skip path green; japicmp green; chaos e2e 4/4; 60s soak smoke green (row guard: 20.8M rows).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR hardens CI and test posture for the Spice Java SDK by adding runtime-backed resilience and TLS/mTLS integration tests, introducing long-running/perf signal coverage, and consolidating CI workflow logic into reusable composite actions.
Changes:
- Adds new test suites/fixtures: chaos E2E against a real
spicedprocess, runtime-generated TLS/mTLS handshake coverage, nightly soak, and benchmark trend emission. - Introduces CI hardening: composite actions for Spice install and runtime startup, nightly workflows, OWASP dependency-check split, and a japicmp API-compatibility gate.
- Exposes keep-alive tuning constants for resilience tests and adds build properties for japicmp/Jacoco-skip scenarios.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/ai/spice/TestFlightSqlServer.java | Adds TLS/mTLS capability to the in-process Flight SQL server fixture. |
| src/test/java/ai/spice/TestCerts.java | Introduces runtime-generated PEM cert fixtures for TLS/mTLS tests. |
| src/test/java/ai/spice/SpicedProcess.java | Adds a real spiced process manager for chaos/lifecycle tests. |
| src/test/java/ai/spice/SoakTest.java | Adds an availability-gated long-running soak test for leaks/latency/thread stability. |
| src/test/java/ai/spice/PerfBenchmarkTest.java | Adds deterministic row-count contracts and JSON benchmark emission for trend tracking. |
| src/test/java/ai/spice/MtlsTest.java | Adds in-process TLS and mutual-TLS handshake integration tests with negative-path assertions. |
| src/test/java/ai/spice/ChaosE2ETest.java | Adds gated chaos E2E tests validating retry/reconnect/keep-alive behaviors against real runtime failures. |
| src/main/java/ai/spice/SpiceClient.java | Exposes keep-alive tuning constants for tests and reuses them in channel configuration. |
| pom.xml | Adds japicmp “old version” property/gate configuration and ensures empty argLine when JaCoCo is skipped. |
| docs/testing.md | Documents the new tiered testing approach and how to run tiers locally. |
| .github/workflows/publish.yaml | Hardens release pipeline (pinned downloads, delayed GPG import) but still has a runtime startup sleep. |
| .github/workflows/nightly.yaml | Adds nightly soak and benchmark-trend publishing workflow. |
| .github/workflows/build.yaml | Consolidates setup/start logic, adds chaos job, adds japicmp gate, and hardens OWASP dependency-check execution. |
| .github/actions/start-spice-app/action.yml | New composite action to start a quickstart runtime and poll /v1/ready instead of sleeping. |
| .github/actions/setup-spice/action.yml | New composite action to install Spice CLI/runtime with retries and verification. |
Suppressed comments (1)
.github/workflows/publish.yaml:78
- This workflow step still uses a fixed sleep for runtime startup. The PR description states build/publish jobs should use the shared /v1/ready poll (start-spice-app) to avoid flakiness; keeping sleep 10 here risks the same intermittent failures and contradicts the stated hardening.
- name: Init and start spice app
run: |
spice init spice_qs
cd spice_qs
# Define the dataset inline rather than fetching spiceai/quickstart from
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Windows: the Spice runtime does not run on native Windows (the CLI's 'spice install' exits 1 with a WSL pointer) — the old workflow silently never had a runtime there. The composite actions now install the CLI only and skip the app start on Windows with an explicit notice; Windows jobs validate the SDK against the in-process suite (live tests self-skip), which is what actually happened before, now stated honestly - OWASP: with no local database (cold cache AND failed update), dependency-check forces a full download regardless of autoUpdate=false and burns the gating step's timeout. The gate now runs only when data exists and emits a loud workflow warning otherwise; with the NVD_API_KEY secret added the gate becomes effectively always-on - CodeQL: explicit least-privilege permissions blocks (contents: read) on all jobs missing them across build/nightly/publish workflows - SpicedProcess.waitUntilHealthy: preserve the interrupt flag and abort the wait instead of swallowing InterruptedException (Copilot) Chaos e2e revalidated 4/4; full gate green.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (3)
.github/workflows/publish.yaml:80
- The PR description says the flaky fixed sleeps in build/publish jobs were retired in favor of the /v1/ready poll, but publish.yaml still starts the runtime with a fixed sleep (later in this step). This leaves the release pipeline exposed to the same readiness flake the composite action was introduced to remove.
- name: Init and start spice app
run: |
spice init spice_qs
cd spice_qs
# Define the dataset inline rather than fetching spiceai/quickstart from
src/test/java/ai/spice/SpicedProcess.java:109
- ProcessBuilder is constructed with findBinary() directly; if no spiced binary is present this passes null as the executable and will throw a NullPointerException with little context. Even though ChaosE2ETest asserts binary presence, this helper should fail fast with a clear message (and avoid surprising NPEs if reused elsewhere).
private void launch() throws Exception {
ProcessBuilder builder = new ProcessBuilder(
findBinary(),
"--http", "127.0.0.1:" + httpPort,
"--flight", "127.0.0.1:" + flightPort,
src/test/java/ai/spice/PerfBenchmarkTest.java:111
- recordBench() assumes that an existing BENCH_JSON file contains a valid JSON array. If the file exists but is empty/whitespace (or partially written from a prior run), JsonParser.parseString(...).getAsJsonArray() will throw and fail the benchmark run. Making the read tolerant of empty content avoids fragile failures in CI/local runs.
private static synchronized void recordBench(String name, String unit, long value) throws Exception {
String path = System.getenv("BENCH_JSON");
if (path == null || path.isEmpty()) {
return;
}
Path file = Path.of(path);
JsonArray entries = Files.exists(file)
? JsonParser.parseString(Files.readString(file)).getAsJsonArray()
: new JsonArray();
A timed-out update leaves a partial .mv.db, which passed the presence check — and dependency-check forces a full re-download over a partial database regardless of autoUpdate=false, burning the gating timeout. The gate now runs only when the update step succeeded (fresh, complete data); otherwise the job emits a loud warning and succeeds — which also lets actions/cache finally persist the update's incremental progress (the cache only saves on job success), so cold caches converge to warm across runs and the gate self-activates.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.github/workflows/build.yaml:215
- The shell snippet builds an EXTRA string and expands it unquoted into the mvn command. If the API key ever contains whitespace or shell metacharacters, this can break argument parsing (and unquoted expansion is generally risky). Use an args array and pass it as "${EXTRA_ARGS[@]}" instead.
run: |
EXTRA=""
if [ -n "$NVD_API_KEY" ]; then
EXTRA="-DnvdApiKey=$NVD_API_KEY"
fi
mvn dependency-check:update-only -B $EXTRA
src/test/java/ai/spice/SpicedProcess.java:109
- SpicedProcess.launch() passes findBinary() directly into ProcessBuilder. If the binary is missing (or the method is reused outside the ChaosE2ETest gate), this will throw a NullPointerException from ProcessBuilder with little context. Fail fast with a clear IllegalStateException when no spiced binary is found.
private void launch() throws Exception {
ProcessBuilder builder = new ProcessBuilder(
findBinary(),
"--http", "127.0.0.1:" + httpPort,
"--flight", "127.0.0.1:" + flightPort,
"--metrics", "127.0.0.1:" + metricsPort);
sgrebnov
approved these changes
Aug 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the P0/P1 gaps from the test-posture assessment: the SDK's resilience features now have end-to-end proof against real runtime failures, mTLS has real handshake coverage, performance is tracked as a trend with deterministic contracts, a nightly soak hunts slow leaks, an API-compatibility gate protects the public surface, and the CI pipeline itself is hardened against the flakes that kept turning PRs red.
P0 — CI pipeline reliability
.github/actions/setup-spice) with retries + binary verification +GITHUB_TOKEN(the JDK-matrix job lacked the token — the source of most installer flakes), replacing six inlined copies in two languages..github/actions/start-spice-app) with a real/v1/readypoll, replacing four copies of the quickstart heredoc — and retiring the flakysleep 10in the pre-existing build/publish jobs as a side effect.nvdValidForHours=24makes warm caches skip the refresh. Wired for anNVD_API_KEYsecret (needs to be added in repo settings; free from NVD).P0 — Chaos e2e (
ChaosE2ETest+e2e-chaosjob, every PR)Manages its own spiced processes (dataset-free pod, no external dependencies) via a new
SpicedProcessfixture and proves against the real runtime:SpiceClient's actual constants), recovery after SIGCONTP1 — mTLS integration (
MtlsTest,TestCerts, TLS mode inTestFlightSqlServer)Runtime-generated EC P-256 CA/server/client certificates, real handshakes in-process: custom-CA trust, full mutual TLS including parameterized queries, and rejection paths that assert the failure is a TLS failure (this immediately caught the negative tests passing via IPv6 connection-refused before any handshake — both sides now pin 127.0.0.1).
P1 — Perf trend + soak + API gate
PerfBenchmarkTestemitscustomSmallerIsBetterJSON (BENCH_JSON); nightly publishes the trend togh-pages(branch created; action pinned to a full commit SHA; runs with-Djacoco.skip=trueso latencies measure the real SDK, not coverage-instrumented classes). Benchmark ops assert full row counts so a result-dropping regression can't publish an "improved" latency.SoakTest, 30 min): 4-worker mixed workload with periodicreset(); asserts zero errors, non-empty results throughout, leak-free close, bounded thread growth from a warm baseline, and p99 stability across the run. Local validation: ~8-9k ops/s, 500k+ ops per minute-long smoke.japicmp.oldVersionis a pom property and the publish preflight fails on a vacuous self-comparison.Security hardening (from adversarial review)
Review chain applied
SpicedProcessextraction, sharedcountRowsreuse, EC certs, dead-code and FQN cleanups; nightly chaos job dropped as a verbatim duplicate of the per-push job.Validation
docs/testing.mddocuments the tiers and how to run each locallyFollow-ups (deferred, noted in review)
NVD_API_KEYrepo secret (speeds the OWASP data refresh)gh-pagesif the benchmark dashboard should be served (data accumulates in-branch regardless)ai.spice.examplefrom the published jar (would obsolete the japicmp exclusion)